Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix revealer using unmap event #187

Merged
merged 4 commits into from
Jan 14, 2019
Merged

Fix revealer using unmap event #187

merged 4 commits into from
Jan 14, 2019

Conversation

peteruithoven
Copy link
Collaborator

Fixes: elementary/wingpanel-indicator-bluetooth#64
Supersedes: #174

After the bluetooth indicator disappears it sometimes doesn't properly re-appear.
This can be reproduced by running:

systemctl restart bluetooth

I can also reproduce the issue by rapidly switching nightlight on off. You see it sometimes not showing up or even showing only half of the icon:
screen record from 2019-01-11 21 20 15
screen record from 2019-01-11 21 19 21
(Using Enter to switch the button makes this easier)

I noticed I could fix this by commenting the delayed re-ordering:
https://github.com/elementary/wingpanel/blob/master/src/Widgets/IndicatorEntry.vala#L82-L85
But this means the indicator isn't removed and you'll see extra margins.

Removing the previous timeout and increasing the interval helps a great deal. But that means another arbitrary value, which could still give issues on slower systems and adds a delayed sudden jump in the indicators.

I was hoping to find a solution like: #155

This PR's uses the unmap signal to know a indicator is hidden, it then start a 0ms timeout to request the menubar reordering. The timeout makes it cancelable when the indicator is quickly made visible again.

Still feels like quite a workaround, I'm open for better solutions.
Disconnecting from the unmap signal when the indicator is made visible again gave issues for some reason.
It could be that a better solution is doing a less crude reordering in the menubar?

@jeremypw
Copy link
Collaborator

I'll need a little time to get up to speed with the code base before reviewing ...

@jeremypw
Copy link
Collaborator

I can confirm that it fixes the problem - I'll have a think about whether a more elegant solution exists.

@jeremypw
Copy link
Collaborator

@peteruithoven : A simple solution that seems to work is to add an Idle loop in IndicatorMenubar.apply_new_order (). Then you can remove all the related Timeouts in Indicator.vala.

@peteruithoven
Copy link
Collaborator Author

I'm afraid that when I remove the timeout stuff and change the apply_new_order to the following, I still have the issue that the indicator sometimes doesn't appear of even only appears half way.

    public void apply_new_order () {
        Idle.add(() => {
            clear ();
            append_all_items ();
            return false;
        });
    }

Maybe there is a better way to make the reorder call cancelable than with a timeout?
Like I mentioned I tried disconnecting from the unmap when the indicator was made visible again, but that caused the Wingpanel to crash when enabling, disabling quickly.

@jeremypw
Copy link
Collaborator

jeremypw commented Jan 14, 2019

@peteruithoven: OK, I guess it must depend on hardware characteristics - I could not trigger the bug using a simple Idle (but I could only occasionally trigger it in master anyway). You could try using Idle.add_full with GLib.Priority.LOW and/or also keep a reference to the source id so that you can cancel the Idle if apply_new_order is called too rapidly. It seems that the fix should be kept within IndicatorMenubar if possible even if it means using a Timeout there.

There are probably more complex solutions using async functions and cancellables and/or mutexes.

@jeremypw
Copy link
Collaborator

Don't forget to install the changes and restart wingpanel - I have sometimes forgotten!

@peteruithoven
Copy link
Collaborator Author

You could try using Idle.add_full with GLib.Priority.LOW and/or also keep a reference to the source id so that you can cancel the Idle if apply_new_order is called too rapidly.

I'm happy to report that that seems to work. Did I implement it like you meant it?

I kind of prevent that mistake by temporarily removing wingpanel from the monitored list of cerbere and using the following the following command to compile and run:
ninja && sudo ninja install && G_MESSAGES_DEBUG=all wingpanel

@jeremypw
Copy link
Collaborator

jeremypw commented Jan 14, 2019

Good that it works! The only minor problem is that it will give critical warnings in the terminal if the source no longer exists when you try to remove it so you need to test whether it is greater than zero before removing it and set it to zero at the end of the Idle closure. The id should also have a default value of zero.

@jeremypw
Copy link
Collaborator

@peteruithoven : Could you try this?

        base_indicator.notify["visible"].connect (() => {
            if (menu_bar != null && (revealer.get_reveal_child () != base_indicator.visible)) {
                /* order will be changed so close all open popovers */
                popover_manager.close ();

                if (base_indicator.visible) {
                    popover_manager.register_indicator (this);
                } else {
                    popover_manager.unregister_indicator (this);
                }

                set_reveal (base_indicator.visible);
                menu_bar.apply_new_order ();
            } else {
                set_reveal (base_indicator.visible);
            }
        });

It works for me.

@jeremypw
Copy link
Collaborator

Ah! I see you have already pushed some changes before I edited my comment - sorry.

@peteruithoven
Copy link
Collaborator Author

@jeremypw thanks I forgot about that error, I've added a check.

You're last suggestion prevents the Revealer hide animation, because it's removed immediately?

@jeremypw
Copy link
Collaborator

Ah, OK. I'll approve as is then.

@jeremypw jeremypw merged commit 65e66c1 into master Jan 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bluetooth icon disapeared
2 participants